Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix preview bin directory for git bash #1009

Conversation

ladevieq
Copy link

@ladevieq ladevieq commented May 3, 2020

Hey thanks for the great work on fzf.

This PR is supposed to fix #988.
If you have suggestions, i can edit the code and test on my Windows computer.

The preview bin is not found on window using git for windows.
The issue is that git bash is using a unix like path architecture
(disk mounted in /mnt dir).

The fix replace, path disk name with unix path to disk style, and \
with /.

The preview bin is not found on window using git for windows.
The issue is that git bash is using a unix like path architecture
(disk mounted in /mnt dir).

The fix replace, path disk name with unix path to disk style, and `\`
with `/`.
@habamax
Copy link
Contributor

habamax commented May 8, 2020

I was skeptical at first... but it works for me. Thx!

image

@junegunn
Copy link
Owner

@janlazo Hi, sorry to bother you but do you think this is a proper fix?

@@ -41,7 +41,8 @@ if s:is_win
else
let s:bin.preview = fnamemodify(s:bin.preview, ':8')
endif
let s:bin.preview = 'bash '.escape(s:bin.preview, '\')
let git_bash_bin = 'bash /mnt/'.substitute(s:bin.preview, '\([A-Z]\):', '\L\1\e', '')
Copy link
Contributor

@janlazo janlazo May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is /mnt/ here? What if git installed with another method? What if the bash executable is not git bash?

Copy link
Author

@ladevieq ladevieq May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best fix would probably be to write a Powershell script that would be use for windows.
This would require to bypass the execution policy, see. I'm not sure this can be done without admin rights.

If you are ok with this, i can create another PR with a Powershell script.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with it but the powershell script should be cross-platform if possible so that it can be tested on Linux.

@habamax
Copy link
Contributor

habamax commented May 11, 2020

Something that doesn't require additional tools to be installed would be nice.
I wonder if it is possible to use plain bat/cmd files or built in js?

@janlazo
Copy link
Contributor

janlazo commented May 11, 2020

What is the shell set in the vimrc, shell options for that shell, and the shell (environment) used to run vim/nvim?

Windows allows forward slash for running binaries via executable path but bash and git need to be executed directly without the user's shell. Preview commands was coded to make it run a temporary batchfile which runs cmd.exe so changing to forward slash can only make things worse.

@@ -41,7 +41,8 @@ if s:is_win
else
let s:bin.preview = fnamemodify(s:bin.preview, ':8')
endif
let s:bin.preview = 'bash '.escape(s:bin.preview, '\')
let git_bash_bin = 'bash /mnt/'.substitute(s:bin.preview, '\([A-Z]\):', '\L\1\e', '')
let s:bin.preview = substitute(git_bash_bin, '\', '/', 'g')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use mixed paths without using cygpath, ok. Problem is if it breaks the shortpath for Vim.

@habamax
Copy link
Contributor

habamax commented May 11, 2020

What is the shell set in the vimrc, shell options for that shell, and the shell (environment) used to run vim/nvim?

All are default in both vim and nvim, what exact option names you have in mind?

shell? shellquote? shellxquote? shellredir?

Windows allows forward slash for running binaries via executable path but bash and git need to be executed directly without the user's shell. Preview commands was coded to make it run a temporary batchfile which runs cmd.exe so changing to forward slash can only make things worse.

But... it directly runs bash with preview.sh as a parameter, did I miss anything?

@janlazo
Copy link
Contributor

janlazo commented May 12, 2020

I wonder if it is possible to use plain bat/cmd files or built in js?

Best to go with powershell if we're considering a different language that must work on Windows. batchfiles are hard to maintain and JS is not available by default.

@janlazo
Copy link
Contributor

janlazo commented May 12, 2020

Preview command should work with vim/nvim defaults especially if the shell options like shellcmdflag and shellslash are left at their default values. It's unusual for it to fail unless there is a regression.

IIRC, fzf-vim passes a preview command to fzf (binary). fzf parses the preview command for a placeholder but the parsing logic is for sh shells. Then, fzf passes the parsed command to the shell. It is always cmd.exe on Windows because SHELL environment variable is no-opt so backward slashes are used for filepaths and passing shortpaths to cmd.exe avoids escaping issues. Using mixed paths and maybe double quotes for shell escape could be sufficient if there is no parsing issue from fzf (binary) because Windows filepath should not have double quotes.

/mnt/ should not be required because git bash has access to the network drives. On my system, sh and bash do not have /mnt/.

@habamax
Copy link
Contributor

habamax commented May 12, 2020

Preview command should work with vim/nvim defaults especially if the shell options like shellcmdflag and shellslash are left at their default values. It's unusual for it to fail unless there is a regression.

Everything is default. And if you look into the issue I have posted results of plain bash calls with preview.sh as parameter. It just doesn't work with full path to preview.sh. #988 (comment)

I suspect the issue is with WSL -- I have it installed and it (probably?) provides bash.exe in system32/ directory. And this bash.exe doesn't work with C:\path\to\preview.sh but works with /mnt/c/path/to/preview.sh

image

@habamax
Copy link
Contributor

habamax commented May 12, 2020

Indeed the issue is with WSL:
image

It does install it in system32 folder and bash from there is found before other bashes.

@janlazo
Copy link
Contributor

janlazo commented May 12, 2020

Is it enough to recommend the user to adjust their PATH so wsl bash is not found first?
fzf.vim can add a warning and exit early or adjust the preview command to use /mnt/${network_drive}/ if wsl bash is found first.

Does WSL bash have a unique text in the output of --version or --help so fzf.vim can parse it?

janlazo added a commit to janlazo/fzf.vim that referenced this pull request May 13, 2020
janlazo added a commit to janlazo/fzf.vim that referenced this pull request May 13, 2020
janlazo added a commit to janlazo/fzf.vim that referenced this pull request May 13, 2020
@habamax
Copy link
Contributor

habamax commented May 13, 2020

Does WSL bash have a unique text in the output of --version or --help so fzf.vim can parse it?

Not sure

PS C:\Users\maksim.kim> bash --version
GNU bash, version 5.0.3(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

PS C:\Users\maksim.kim> bash --help
GNU bash, version 5.0.3(1)-release-(x86_64-pc-linux-gnu)
Usage:  /bin/bash [GNU long option] [option] ...
        /bin/bash [GNU long option] [option] script-file ...
GNU long options:
        --debug
        --debugger
        --dump-po-strings
        --dump-strings
        --help
        --init-file
        --login
        --noediting
        --noprofile
        --norc
        --posix
        --pretty-print
        --rcfile
        --restricted
        --verbose
        --version
Shell options:
        -ilrsD or -c command or -O shopt_option         (invocation only)
        -abefhkmnptuvxBCHP or -o option
Type `/bin/bash -c "help set"' for more information about shell options.
Type `/bin/bash -c help' for more information about shell builtin commands.
Use the `bashbug' command to report bugs.

bash home page: <http://www.gnu.org/software/bash>
General help using GNU software: <http://www.gnu.org/gethelp/>

@habamax
Copy link
Contributor

habamax commented May 13, 2020

Is it enough to recommend the user to adjust their PATH so wsl bash is not found first?

I don't think so. It would be quite cumbersome -- my git and other tools paths are managed by scoop for example. Also I am not sure if wsl would work if I I put wsl bash path after git bash.

@habamax
Copy link
Contributor

habamax commented May 13, 2020

Does WSL bash have a unique text in the output of --version or --help so fzf.vim can parse it?

What I know: bash in wsl has $WSLENV set (but has no value)

according to https://stackoverflow.com/questions/3601515/how-to-check-if-a-variable-is-set-in-bash you can check it in bash with:

if [ -z ${WSLENV+x} ]; then echo "WSLENV is unset"; else echo "WSLENV is set to '$WSLENV'"; fi

I don't know how to invoke it from command line though.

@ladevieq ladevieq closed this May 22, 2020
junegunn pushed a commit that referenced this pull request May 23, 2020
* [fzf#vim#with_preview] support wsl bash

Close #988
Close #1009

* [fzf#vim#with_preview] use abs path for bin/

WSL has issues with relative filepaths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preview doesn't work on Windows
4 participants